Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve logger Pause handling #24946

Merged
merged 3 commits into from
May 27, 2023
Merged

Improve logger Pause handling #24946

merged 3 commits into from
May 27, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented May 26, 2023 •
edited
Loading

The old EventWriter's Run does:

for {
    handlePause()
    select {
    case event <- Queue:
         write the log event ...
    }
}

So, if an event writer is started before the logger is paused, there is a chance that the logger isn't paused for the first message.

The new logic is:

for {
    select {
    case event <- Queue:
         handlePause()
         write the log event ...
    }
}

Then the event writer can be correctly paused

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 26, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 26, 2023
@wxiaoguang wxiaoguang added type/testing skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels May 26, 2023
@wxiaoguang wxiaoguang added this to the 1.20.0 milestone May 26, 2023
@wxiaoguang wxiaoguang changed the title Improve logger PauseAll test Improve logger Pause handling May 26, 2023
@wxiaoguang wxiaoguang force-pushed the fix-fragile-logger-test branch 2 times, most recently from 1305edf to 0062d93 Compare May 26, 2023 13:30
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 26, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented May 26, 2023 •
edited
Loading

Last CI failure is still related to pin issue:

 --- FAIL: TestAPIListPinnedIssues (1.58s)
    testlogger.go:60: 2023/05/26 13:44:33 ...les/storage/local.go:47:NewLocalStorage() [I] Creating new Local Storage at /home/runner/work/gitea/gitea/tests/gitea-lfs-meta
    testlogger.go:60: 2023/05/26 13:44:33 ...eb/routing/logger.go:102:func1() [I] router: completed GET /user/login for test-mock:12345, 200 OK in 7.9ms @ auth/auth.go:141(auth.SignIn)
    testlogger.go:60: 2023/05/26 13:44:33 ...eb/routing/logger.go:102:func1() [I] router: completed POST /user/login for test-mock:12345, 303 See Other in 9.8ms @ auth/auth.go:170(auth.SignInPost)
    testlogger.go:60: 2023/05/26 13:44:34 ...eb/routing/logger.go:102:func1() [I] router: completed GET /user/settings/applications for test-mock:12345, 200 OK in 20.6ms @ setting/applications.go:23(setting.Applications)
    testlogger.go:60: 2023/05/26 13:44:34 ...eb/routing/logger.go:102:func1() [I] router: completed POST /user/settings/applications for test-mock:12345, 303 See Other in 126.1ms @ setting/applications.go:33(setting.ApplicationsPost)
    testlogger.go:60: 2023/05/26 13:44:34 ...eb/routing/logger.go:102:func1() [I] router: completed GET /user/settings/applications for test-mock:12345, 200 OK in 22.7ms @ setting/applications.go:23(setting.Applications)
    testlogger.go:60: 2023/05/26 13:44:34 ...eb/routing/logger.go:102:func1() [I] router: completed POST /api/v1/repos/user2/repo1/issues/2/pin?token=a36803b2e14811c9a8fffed3cbd319d55cf7963e for test-mock:12345, 204 No Content in 131.2ms @ repo/issue_pin.go:16(repo.PinIssue)
    testlogger.go:60: 2023/05/26 13:44:34 ...eb/routing/logger.go:102:func1() [I] router: completed GET /api/v1/repos/user2/repo1/issues/pinned for test-mock:12345, 200 OK in 7.9ms @ repo/issue_pin.go:175(repo.ListPinnedIssues)
    api_issue_pin_test.go:170: 
        	Error Trace:	/home/runner/work/gitea/gitea/tests/integration/api_issue_pin_test.go:170
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestAPIListPinnedIssues
panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0

goroutine 24092 [running]:
testing.tRunner.func1.2({0x45fb980, 0xc00218d968})
	/opt/hostedtoolcache/go/1.20.4/x64/src/testing/testing.go:1526 +0x372
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.20.4/x64/src/testing/testing.go:1529 +0x650
panic({0x45fb980, 0xc00218d968})
	/opt/hostedtoolcache/go/1.20.4/x64/src/runtime/panic.go:890 +0x263
code.gitea.io/gitea/tests/integration.TestAPIListPinnedIssues(0x0?)
	/home/runner/work/gitea/gitea/tests/integration/api_issue_pin_test.go:171 +0x799
testing.tRunner(0xc0486c01a0, 0x4c93198)
	/opt/hostedtoolcache/go/1.20.4/x64/src/testing/testing.go:1576 +0x217
created by testing.(*T).Run
	/opt/hostedtoolcache/go/1.20.4/x64/src/testing/testing.go:1629 +0x806
make[1]: *** [/home/runner/work/gitea/gitea/Makefile:572: test-pgsql] Error 2
make: *** [Makefile:8: test-pgsql] Error 2
Error: Process completed with exit code 2.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 27, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 27, 2023
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2023
@silverwind silverwind merged commit 0d54395 into go-gitea:main May 27, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2023
@wxiaoguang wxiaoguang deleted the fix-fragile-logger-test branch May 28, 2023 02:54
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 29, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
4825cb5 
* upstream/main:
  Test query must have "order by" explicitly to avoid unstable results (go-gitea#24963)
  Help to recover from corrupted levelqueue (go-gitea#24912)
  [skip ci] Updated translations via Crowdin
  Remove meta tags `theme-color` and `default-theme` (go-gitea#24960)
  Add dark mode to API Docs (go-gitea#24971)
  Update JS dependencies (go-gitea#24969)
  Replace Fomantic reset module with our own (go-gitea#24948)
  simple docs fixes: 'pull request' page (en-us & zh-tw) link path to 'issue-pull-request-templates' (go-gitea#24961)
  Remove reference to caddy v1 in docs (go-gitea#24962)
  Improve and fix bugs surrounding reactions (go-gitea#24760)
  Use `[git.config]` for reflog cleaning up (go-gitea#24958)
  Improve logger Pause handling (go-gitea#24946)
  Do not output "Trace" level logs from process manager by default (go-gitea#24952)
  Make the 500 page load themes (go-gitea#24953)
  [skip ci] Updated translations via Crowdin
  docs: remove an extraneous whitespace (go-gitea#24949)
  Show `bot` label next to username when rendering autor link if the user is a bot (go-gitea#24943)
  Improve some Forms (go-gitea#24878)
  Improve queue and logger context (go-gitea#24924)
  Fix ref type error (go-gitea#24941)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@silverwind silverwind silverwind approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/testing
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.
4 participants
@wxiaoguang @lunny @silverwind @GiteaBot